Set default SVG currentColor to black#3400
Conversation
|
Cant get the build run locally, and jenkins reporting build errors, maybe someone can take a look? Thanks |
|
Don't do merge commits; we can't merge this into master in the end because we don't accept merge commits. In fact don't do multiple commits in general. Use rebase. Amend you commit and force push it when you need to make subsequent follow up changes. You'll need to rebase and force push to repair this PR. |
|
I'll approve the workflow so you can see any errors in the GitHub actions. Please rebase and force push to get this back on track. |
|
See we even fail with a check for merge commits: 
|
DLTKJoelJohansson
force-pushed
the
svg-fg-color
branch
from
a2d55a0 to
e05c477
Compare
|
I've approved the workflow again. I'll be out of office for a few hours... |
DLTKJoelJohansson
force-pushed
the
svg-fg-color
branch
from
e05c477 to
952e38e
Compare
|
Cant seem to figure it out, its failing for Linux builds |
DLTKJoelJohansson
force-pushed
the
svg-fg-color
branch
2 times, most recently
from
8aa4474 to
34c413c
Compare
|
Okay got it to work now! Moved out the Display.getDefault() call to org.eclipse.swt bundle (In SVGFileFormat, the caller) fixed it. Thanks @merks for your help. Let me know what you think |
|
Maybe not, seems like the Browser tests for Windows are failing. Pretty strange |
|
@merks any thoughts on this? I have a hard time seeing how these changes can cause the Browser tests to fail on Windows, since the changes only touch the SVG Rasterization |
|
It could just be a flaky test. I'll rerun it. |
|
Looks like the same Browser tests are failing again, but at least the build succeeded now |
|
I've not looked closely, but is the failure of the browser test on Windows related to this PR or maybe known to be flaky? |
|
Thanks @HeikoKlare, and @merks! This PR should ready for review now |
|
I will leave it to someone qualified to review the changes. |
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
Thank you for proposing a solution to the mentioned issue. I have some concerns regarding the proposed solution (please see the detailed comment). What could also help is an example (e.g., a snippet or an RCP extension) that demonstrates the behavior and the change, so that we can easily test different alternatives.
DLTKJoelJohansson
force-pushed
the
svg-fg-color
branch
from
7091da9 to
74652a7
Compare
DLTKJoelJohansson
changed the title
|
Thanks @HeikoKlare for the feedback. I made the change so now we simply set the default foreground color to black, reverting back to the behaviour it was before the change in the underlying renderer. I also added a regression test as you proposed. I am just checking one pixel seeing if its black. I guess you could test all pixels but I kinda felt it was overkill. Maybe there is an easier way to test this, but I could not find any. Jenkins are failing too, not sure why. From the info I can see the compiler is saying |
DLTKJoelJohansson
force-pushed
the
svg-fg-color
branch
from
74652a7 to
b7a37fb
Compare
|
@HeikoKlare, any thoughts on this? It might not be the compiler that is failing the build, but I cannot see any details |
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
Thank you for updating the fix with this simple restoration of the previous behavior. We identified just today that our product is also affected by this issue, so I directly tested the proposed fix with it. I can confirm that is properly restores the previous behavior.
The test also looks sound to me. Just testing a single pixel is absolutely sufficient.
The reported warnings in the failing Jenkins check are strange. They are obviously not introduced by this PR. I have restarted the Jenkins build. Let's see if the next run produces proper results.
|
Thank you, great! I see that the build is still giving the same results |
|
I have retriggered an SWT 
Only difference I see between the previous and the current build is a newer ECJ version, but I did not find any suspicious changes at ECJ. Anyway, the new warnings are not related to this PR and there is no other degradation reported in the build, so I will merge it as is. |
4 participants
An attempt at fixing #3398.
Let me know what you think.
Thanks!